Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bump kubernetes- and openshift-client to 7.0.0 (#247) #248

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

adietish
Copy link
Contributor

@adietish adietish commented Oct 8, 2024

fixes #247

@adietish adietish self-assigned this Oct 8, 2024
@adietish adietish changed the title fix: bump kubernetes- and openshift-client to 7.0-SNAPSHOT (#247) fix: bump kubernetes- and openshift-client to 7.0.0 (#247) Dec 3, 2024
@adietish
Copy link
Contributor Author

adietish commented Dec 3, 2024

kubernetes-client 7.0 was released: https://repo1.maven.org/maven2/io/fabric8/kubernetes-client/7.0.0/

@adietish adietish force-pushed the issue-247 branch 6 times, most recently from 6482dba to ce7e878 Compare December 13, 2024 11:07
@adietish
Copy link
Contributor Author

weird enough tests only fail when run on IC-2024.2...

@@ -38,30 +38,22 @@ public class ConfigWatcher implements Runnable {
private WatchService service;

public interface Listener {
void onUpdate(ConfigWatcher source, Config config);
void onUpdate(Config updatedConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigWatcher now notifies changes in the client config (which is the result of merging all config files)

public ConfigWatcher(String config, Listener listener) {
this(Paths.get(config), listener);
public ConfigWatcher(Listener listener) {
this(Config.getKubeconfigFilenames().stream().map(Paths::get).toList(), listener, new HighSensitivityRegistrar());
Copy link
Contributor Author

@adietish adietish Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigWatcher does not need to get the path to the config file. It is using the Client config which merged all the config files that were merged when it (auto-) configured
One could have provided ConfigWatcher with a list of paths instead of it relying on kubernetes-client class Config (avoiding this dependency). But the client Config is the class that is notified in case of changes in any of the kubeconfs. So there's no "avoiding" it.

this.configs = configs;
this.listener = listener;
this.registrar = registrar;
}

@Override
public void run() {
watch((Config config) -> listener.onUpdate(this, config));
watch(listener::onUpdate);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notify listeners of a change in the client config (not individual kubeconf files)

@@ -70,11 +62,11 @@ public void close() throws IOException {
}
}

private void watch(Consumer<Config> consumer) {
private void watch(Consumer<Config> listener) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple rename, "listener" is more precise.

for (WatchKey key = service.take(); key != null; key = service.take()) {
key.pollEvents().forEach((event) -> {
Path changed = getAbsolutePath(directory, (Path) event.context());
if (isConfigPath(changed)) {
consumer.accept(loadConfig(changed));
Config config = new ConfigBuilder().build();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont load a particular kubeconf. Have the kubernetes-client load and merge all the files that are configured.

* Contributors:
* Red Hat, Inc. - initial API and implementation
******************************************************************************/
package com.redhat.devtools.intellij.common.utils;
Copy link
Contributor Author

@adietish adietish Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved methods that deal with the tools config in ConfigHelper to a new class ToolsConfigHelper. ConfigHelper now onlay has methods that deal with the kubernernetes-client config.

@adietish
Copy link
Contributor Author

@sbouchet: added comments explaining why I introduced a change

@adietish
Copy link
Contributor Author

adietish commented Dec 13, 2024

@sbouchet:

weird enough tests only fail when run on IC-2024.2...

Bumping mockito to 5.2.0 fixes the issue

Didnt find out why IC-2024.2 (and only this version, not latter 24.3) is running tests using jdk21 even though the build specifies jdk17.
The issue is that byte buddy within mockito 4.x wont support classes that are compiled with jdk21.

Caused by:
    java.lang.IllegalArgumentException: Java 21 (65) is not supported by the current version of Byte Buddy which officially supports Java 19 (63) - update Byte Buddy or set net.bytebuddy.experimental as a VM property
    at net.bytebuddy.utility.OpenedClassReader.of(OpenedClassReader.java:96)

Mockito 5.2.0 includes a version of the byte buddy that supports jdk21.

@redhat-developer redhat-developer deleted a comment from sonarqubecloud bot Dec 13, 2024
@adietish adietish force-pushed the issue-247 branch 2 times, most recently from 23fa60c to 5ced17e Compare December 20, 2024 16:43
@adietish adietish force-pushed the issue-247 branch 4 times, most recently from 4233ac8 to 4b9fe5c Compare January 7, 2025 16:28
@adietish adietish marked this pull request as ready for review January 7, 2025 16:31
@adietish adietish requested a review from sbouchet January 7, 2025 16:31
@adietish
Copy link
Contributor Author

adietish commented Jan 7, 2025

@sbouchet: This PR for intellij-common is ready. Please review.

@adietish adietish force-pushed the issue-247 branch 2 times, most recently from 0afd96d to 347244a Compare January 8, 2025 13:19
@@ -3,7 +3,7 @@ nexusUser=invalid
nexusPassword=invalid

# IntelliJ Platform Properties -> https://plugins.jetbrains.com/docs/intellij/tools-gradle-intellij-plugin.html#configuration-intellij-extension
ideaVersion=2024.2
ideaVersion=2024.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also update the ci to validate against this version ( i know it's this one used in OS-matrix build, but this will provide us verifier reports )

Copy link
Contributor

@sbouchet sbouchet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beside ci workflow update that can be done in anoher PR, LGTM.

* changed API for ConfigWatcher#Listener#onUpdate
* removed references to specific config file, use files collected by client instead
* bumped mockito to support jdk21

Signed-off-by: Andre Dietisheim <[email protected]>
Copy link

sonarqubecloud bot commented Jan 9, 2025

@adietish adietish merged commit 109628c into redhat-developer:main Jan 9, 2025
14 checks passed
@adietish adietish deleted the issue-247 branch January 9, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump kubernetes-client to 7.0.0
2 participants